nixos: remove optional builtins prefixes from prelude functions#447403
Conversation
f15a8c0 to
6bc130c
Compare
qweered
left a comment
There was a problem hiding this comment.
Skimmed through changes, nothing questionable
Seeing as no one seems to be objecting, I'm happy to re-review and merge once rebased. FYI #480141 is also pulling in the new nixd linting rules into our ci-pinned nixpkgs, although it looks like we'll be disabling them at first to avoid a treewide cleanup. |
Remove optional builtins prefixes from prelude functions by running:
builtins=(
abort
baseNameOf
break
derivation
derivationStrict
dirOf
false
fetchGit
fetchMercurial
fetchTarball
fetchTree
fromTOML
import
isNull
map
null
placeholder
removeAttrs
scopedImport
throw
toString
true
)
fd \
--exclude doc/manual/release-notes \
--type file \
. \
nixos \
--exec-batch sed --in-place --regexp-extended "
s/\<builtins\.($(
printf '%s\n' "${builtins[@]}" |
paste --delimiter '|' --serial -
))\>/\1/g
"
nix fmt
dc9c0ac to
a2ed7e8
Compare
I updated this PR exactly like in #447403 (comment). Like before, here is the relevant diff against the previous diff:
|
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
I skimmed the diff very quickly again. No obvious issues.
Rebuild count isn't particularly relevant to NixOS modules, but only seeing two is still encouraging:
I'll run a nixpkgs-review just to be safe (not sure if nixpkgs-review-gha will be able to build tests.nixos-functions.nixos-test, but we'll see). EDIT: ah, it's blacklisted.
I aim to get this merged promptly before the inevitable conflicts reappear. If issues are found post-merge we can fix or revert as necessary.
Built fine locally |
|
This needs to be backported or else other backports are going to be difficult. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-25.11
git worktree add -d .worktree/backport-447403-to-release-25.11 origin/release-25.11
cd .worktree/backport-447403-to-release-25.11
git switch --create backport-447403-to-release-25.11
git cherry-pick -x a2ed7e8d882c2b9deba7b7c2cc8e1a97b29aa415 |
|
@trueNAHO: Can you create a new PR for the release-25.11 branch, please? (I presume since it's auto-generated that it might be easier to do it that way than to cherry-pick and resolve conflicts.) |
Good point. This applies equally to the other PRs in this series:
We asked for the initial PRs to be split for easier review, however I'm fine to review a single backport.
If the backport is made of 5 commits (freshly generated, but still tagged with |
|
Sure, do whatever feels best 😃 |
I opened #480781.
Based on the following output, the other PRs were already merged prior to the $ for commit in \
b50da098117870e48cba6c110d290cb2a2dadf93 \
44998896c6a811fcc4021e8ca75243b5d53e605b \
6d1d15bb3610a98c8d6ba63e7e792cec53215227 \
c078ee646535e63a08f709d3f1b5d122407e6f12 \
d9335d5dd7057963bf0006432427e115b2825c4d
do
git merge-base --is-ancestor "$commit" release-25.11 || echo "$commit"
done
c078ee646535e63a08f709d3f1b5d122407e6f12
Are you referring to some general GitHub feature, or just #480781 (review)? I usually run something like the following locally: diff <(git show "$COMMIT_1") <(git show "$COMMIT_2") |
I was refering to that review that our CI posts, yes. GitHub doesn't have any native features for inspection backports and comparing them to the original.
That's essentially what our CI does. It relies on the backport having a |
Related PRs intentionally split up to simplify review and merging:
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.